Add Stackdriver Trace V2 models by liyanhui1228 · Pull Request #87 · census-instrumentation/opencensus-python

liyanhui1228 · 2017-11-28T23:06:49Z

chingor13

Are you missing the MessageEvent model?

Also, it seems that this PR is just finishing implementing the OpenCensus trace.proto models as they aren't being used in the StackdriverExporter yet.

chingor13 · 2017-11-29T22:48:24Z

+
+def _format_attribute_value(value):
+    if type(value).__name__ == 'str':
+        value_type = 'string_value'


The string_value value is a TruncatableString which has a value field.

Good catch! Done.

For Python 2/3 compatibility, we could check for bytes, str, and unicode, and convert them to str using 'utf-8' encoding. Just doing str might be good enough for now, though.`

liyanhui1228 · 2017-11-30T19:49:52Z

Yes will change the StackdriverExporter to use the models in later commits.

…census-python into stackdriver-v2

…encensus-python into stackdriver-v2

duggelz · 2017-12-02T00:35:25Z

+    if type(value).__name__ == 'str':
+        value_type = 'string_value'
+        value = _get_truncatable_str(value)
+    elif type(value).__name__ == 'int':


Can just do if isinstance(value, int) I think. Same for bool and str

isinstance(True, int) will also return True. Changed to put the check for bool at the first if statement and check int later.

duggelz · 2017-12-02T00:37:18Z

+
+def _format_attribute_value(value):
+    if type(value).__name__ == 'str':
+        value_type = 'string_value'


For Python 2/3 compatibility, we could check for bytes, str, and unicode, and convert them to str using 'utf-8' encoding. Just doing str might be good enough for now, though.`

duggelz · 2017-12-02T00:38:34Z

+
+    :type attributes: dict
+    :param attributes: The set of attributes. Each attribute's key can be up
+                       to 128 bytes long. The value can be a string up to 256


Should we check and enforce these limits in the code?

Added check for length of the attribute key. The length of string value is checked in opencensus.trace.utils._get_truncatable_str when converting to TruncatableString.

duggelz · 2017-12-02T00:39:37Z

+
+        self.attributes = attributes
+
+    def set_attribute(self, key, value):


Is there any need for a "delete attribute" in the API? I think probably no, but I wanted to mention it.

duggelz · 2017-12-02T00:41:46Z

+        if attributes is None:
+            attributes = {}
+
+        self.attributes = attributes


We should probably copy the attributes dictionary instead of taking it by reference, so that the caller can't accidentally change our internal dictionary. E.g. something like

self.attributes = dict(attributes or {})

This is not absolutely required, but it's a good pattern to follow in general, and especially in constructors, to avoid two pieces of code sharing a list or dict without realizing it.

Good catch! Done.

duggelz · 2017-12-02T00:43:46Z

+            value = self.attributes.get(key)
+            value_json = _format_attribute_value(value)
+            attributes_json[key] = value_json
+


Not required, but it you like, you could also do this:

attributes_json = {key: _format_attribute_value(value) for key, value in self.attributes}

duggelz · 2017-12-02T00:44:56Z

        """
-        :type trace: dict
-        :param trace: Trace collected.
+        :type spans: dict


Should this be a private method? Otherwise, we should probably explain the translation a bit in the comment.

duggelz · 2017-12-02T00:52:36Z


+    def add_time_event(self, time_event):
+        """Add a TimeEvent."""
+        self.time_events.append(time_event)


We might want to check that this is a TimeEvent and throw TypeError otherwise. Same as link below.

duggelz · 2017-12-05T06:54:50Z

You are going to have a massive merge conflict between this and the big rename. Which one are you going to do first?

liyanhui1228 · 2017-12-05T07:53:07Z

The Stackdriver V2 won't be included in the first release. So I'll merge the restructure PR first.

duggelz · 2017-12-05T07:48:53Z

+        self.attributes = attributes
+
+    def format_link_json(self):
+        """Conver a Link object to json format."""


Conver -> Convert

duggelz · 2017-12-05T08:04:41Z

+    count.
+    """
+    str_bytes = str_to_convert.encode(UTF8)
+    str_len = len(str_bytes)


This seems kind of problematic, because you could convert a code point to a 2 or 3 byte utf-8 sequence, and then truncate the string in the middle of that sequence, creating an invalid string that causing an exception to be thrown.

But I think you can get around this by changing the code below to

truncated = str_bytes.decode(UTF8, errors='ignore')

This seemed to do the right thing in both Python 2 and 3. Can you add a unit test where you call this on a string with 127 ascii characters followed by a some non-ascii characters?

I used the following code to play with truncation: ('\u2603\u2603'.encode('utf-8')[0:5]).decode('utf-8', errors='ignore').

Thanks for the explanation! Added the unit test for that.

duggelz · 2017-12-05T08:05:57Z

+    if str_len > limit:
+        str_bytes = str_bytes[:limit]
+
+    result = str_bytes.decode(UTF8)


Same comment here as above. Perhaps one of these functions could call the other, or have a helper function that takes a (string, limit) and returns (truncated_string, truncated_byte_count)?

Good suggestion, done.

…encensus-python into stackdriver-v2

…census-python into stackdriver-v2

duggelz

LGTM except I'm curious about the randint change

duggelz · 2017-12-08T02:00:43Z

              than 0 and unique within a trace.
    """
-    span_id = random.getrandbits(64)
+    span_id = random.randint(10**15, 10**16 - 1)


Why did this change?

Because the V2 API only accepts 16-digit span_id, but we can also trim the span_id in the stackdriver_exporter.

Ok. Maybe document that rationale, and/or make those named constants.

duggelz · 2017-12-08T02:02:14Z

+    :returns: The string it self if not exceeded length, or truncated string
+              if exceeded and the truncated byte count.
+    """
+    if limit is None:


Could also just do limit=MAX_LENGTH above in the function signature.

liyanhui1228 added 2 commits November 28, 2017 15:04

Add Stackdriver Trace V2 models

43f8dbe

Add unit tests

16f51bf

liyanhui1228 changed the title ~~[WIP] Add Stackdriver Trace V2 models~~ Add Stackdriver Trace V2 models Nov 29, 2017

liyanhui1228 requested a review from chingor13 November 29, 2017 02:10

Fix coverage

432d178

liyanhui1228 added the do not merge label Nov 29, 2017

chingor13 reviewed Nov 29, 2017

View reviewed changes

liyanhui1228 added 3 commits November 29, 2017 18:08

String value in attributes should be truncatable_str

ead6386

Add MessageEvent model

bbbcda3

Merge branch 'master' into stackdriver-v2

00e8247

liyanhui1228 added 2 commits November 30, 2017 17:44

[WIP] Update StackdriverExporter to use V2 API

c5ebe8f

Merge branch 'stackdriver-v2' of https://github.com/liyanhui1228/open…

6b4045c

…census-python into stackdriver-v2

liyanhui1228 changed the title ~~Add Stackdriver Trace V2 models~~ [WIP] Add Stackdriver Trace V2 models Dec 1, 2017

Merge branch 'master' of https://github.com/census-instrumentation/op…

ea4f5e7

…encensus-python into stackdriver-v2

duggelz reviewed Dec 2, 2017

View reviewed changes

liyanhui1228 added 2 commits December 1, 2017 17:37

[WIP] Update StackdriverExporter to use V2

ef408ca

Address comments

6cd5b80

duggelz suggested changes Dec 5, 2017

View reviewed changes

liyanhui1228 added 9 commits December 5, 2017 14:49

Merge branch 'master' of https://github.com/census-instrumentation/op…

c87b17a

…encensus-python into stackdriver-v2

[WIP] Add unit tests

9d6da02

Ignore errors when truncating non-ascii character

89a144d

Merge branch 'master' into stackdriver-v2

743427b

Fix system tests

98d22fb

Merge branch 'stackdriver-v2' of https://github.com/liyanhui1228/open…

2fe086e

…census-python into stackdriver-v2

fix

0d9e60d

[WIP] Update to use V2 API

d55bc09

Update system tests

c0dc5f8

liyanhui1228 changed the title ~~[WIP] Add Stackdriver Trace V2 models~~ Add Stackdriver Trace V2 models Dec 7, 2017

liyanhui1228 removed the do not merge label Dec 7, 2017

duggelz reviewed Dec 8, 2017

View reviewed changes

duggelz approved these changes Dec 8, 2017

View reviewed changes

Address comments

6ddb0b1

liyanhui1228 merged commit 40af615 into census-instrumentation:master Dec 8, 2017

liyanhui1228 deleted the stackdriver-v2 branch December 8, 2017 17:53


		self.attributes = attributes

		def set_attribute(self, key, value):

Conversation

liyanhui1228 commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chingor13 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyanhui1228 commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duggelz commented Dec 5, 2017

Uh oh!

liyanhui1228 commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duggelz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

liyanhui1228 commented Nov 28, 2017 •

edited

Loading

chingor13 left a comment •

edited

Loading

liyanhui1228 commented Nov 30, 2017 •

edited

Loading